Skip to content

Conversation

arrdem
Copy link
Collaborator

@arrdem arrdem commented Sep 23, 2025

As reported by customers, the naive but correct strategy of using copies in py_venv_* can lead to laughable disk usage. Some clients are reporting order 10min slowdowns and order 100GiB disk usage wasted copying inputs into binaries. We need a more scalable strategy such as symlinking.

Thankfully we can generate symlinks from tools driven by Bazel into a TreeArtifact so long as the symlinks aren't dangling. By carefully crafting relative symlinks we're able to produce a tree of links which is valid both at and after action time. When relocating a .runfiles tree containing such links (for instance into a OCI later tar) these links must be dereferenced but that Just Works.

While I'm at it, refactor the venv machinery to operate in terms of strategies and combinators on strategies so that it's simpler to talk about the production-grade behavior we want which is:

  • site-packages trees in 1stparty code get relocated/linked into the venv
  • bin sibling trees in 1stparty code get relocated/patched into the venv
  • General trees in 1stparty code are referred to by .pth file entries
  • General trees in 3rdparty code get relocated/linked into the venv
  • bin sibling trees in 3rdparty code get relocated/patched into the venv

This makes the venv builder significantly more flexible, allows for better error reporting and opens the door to more flexible error handling.

Incorporates an implementation of #606, but testing is required.
Should include an implementation of #635, but testing is required.

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

py_venv_* now use symlinks rather than hard file copies which radically reduce disk usage while improving venv building performance.

Test plan

  • Covered by existing test cases
  • New test cases added
  • Manual testing; please provide instructions so we can reproduce:
    TODO.

Remaining work

  • Strip debug prints
  • Improve collision handling
  • Rework the command interpreter to implement the last-wins semantics
  • Mitigate spooky dangling symlink issues
  • Fix a regression which can cause a site-packages/__init__.py file to be linked
  • Add sha256-sum based collision ignoring
  • Add a test covering that a site-packages/__init__.py file will not be linked
  • Add a test covering bin shebang patching
  • Integrate the test case from fix(py_venv): Repair external repository imports #635
  • Manually test that linked venvs still work; should just be fine

- Refactor the new venv builder to use modular strategies
- Implement copying and symlinking among other strategies
- Implement some strategy combinators
- Refactor the venv tool so only new .pth and symlink strategies are usable
- Implement a _good_ symlink + patching strategy via combinators
- Make the previous copying behavior unreachable
@arrdem arrdem assigned arrdem and myrrlyn and unassigned arrdem Sep 23, 2025
Copy link

aspect-workflows bot commented Sep 23, 2025

Test

All tests were cache hits

45 tests (100.0%) were fully cached saving 1m 28s.

@arrdem arrdem requested a review from myrrlyn September 23, 2025 19:05
@arrdem arrdem assigned arrdem and unassigned myrrlyn Sep 23, 2025
Copy link

github-actions bot commented Sep 23, 2025

e2e/use_release folder: LCOV of commit d62b49f during CI #1990

Summary coverage rate:
  lines......: 100.0% (2 of 2 lines)
  functions..: 100.0% (1 of 1 function)
  branches...: no data found

Files changed coverage rate: n/a

@myrrlyn
Copy link
Contributor

myrrlyn commented Sep 25, 2025

I'm having trouble running the tests on my machine; we can talk about it synchronously later. The meat of your question to me, I think, is about dyn-compatibility and how we can make the source-code simpler when branching on different types.

Short answer: Virtualization is mutually exclusive with generics. fn plan<A: AsRef<Path>, B: AsRef<Path>>(&self, ...) is not virtualizable because it has generics, not because of anything else in the arguments. If you strip the generics out of it, and take &Path in place of the type parameters, it virtualizes just fine.

Especially for functions where you only genericize because you're widening your Postel acceptance, you should get in the habit of writing your primary logic with a concretely lowered type (here &Path), and either explicitly converting at the call site (.as_ref()) or writing a wrapper function that serves as your "true API" with the generics which lowers and then calls the primary logic function.

The reason for this is that the ENTIRE function body of a generic function gets monomorphized for EVERY set of type arguments you use in it. If you have an inner non-generic function, it only exists once (function items are always global to the translation unit, even if they are syntactically nested inside another function; the only thing that does is affect symbol visibility in the privacy system), so the outer function with generics only monomorphizes as a couple function calls.

I'll put up a child PR into this one that illustrates switching the PthEntryHandler over to a virtualizable signature, virtualizing it into Box<dyn PthEntryHandler> in your use sites in venv_bin::venv_cmd_handler, and writing populate_venv to take &dyn PthEntryHandler instead of impl PthEntryHandler in my morning.

I also have style notes on other aspects of the code but I'll save that for after the virtualization.

@myrrlyn
Copy link
Contributor

myrrlyn commented Sep 25, 2025

Ok I'm not tired yet. See #648

myrrlyn and others added 2 commits September 25, 2025 11:25
Code review per request

---------

Co-authored-by: Reid D. McKenzie <[email protected]>
@arrdem arrdem enabled auto-merge (squash) September 25, 2025 18:18
@arrdem arrdem disabled auto-merge September 25, 2025 18:26
@arrdem arrdem merged commit e8dd503 into main Sep 25, 2025
16 checks passed
@arrdem arrdem deleted the arrdem/tweak-symlink-venvs branch September 25, 2025 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants